Skip to content

Antalya 26:6 Fix alter operations for iceberg - #2157

Open
subkanthi wants to merge 25 commits into
antalya-26.6from
antalya_26_6_fix_alter_table_iceberg
Open

Antalya 26:6 Fix alter operations for iceberg#2157
subkanthi wants to merge 25 commits into
antalya-26.6from
antalya_26_6_fix_alter_table_iceberg

Conversation

@subkanthi

@subkanthi subkanthi commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

continuation of work from #1841

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

...

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • Aarch64 tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • OAuth (5m)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Workflow [PR], commit [5968163]

@subkanthi subkanthi changed the title Fix alter operations for iceberg Antalya 26:6 Fix alter operations for iceberg Aug 6, 2026
@subkanthi
subkanthi marked this pull request as ready for review August 6, 2026 04:26
@subkanthi subkanthi closed this Aug 6, 2026
@subkanthi subkanthi reopened this Aug 6, 2026
@subkanthi subkanthi closed this Aug 6, 2026
@subkanthi subkanthi reopened this Aug 6, 2026
@subkanthi subkanthi closed this Aug 6, 2026
@subkanthi subkanthi reopened this Aug 6, 2026
@subkanthi

subkanthi commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

ADD COLUMN

Ubuntu-2404-noble-amd64-base :) ALTER table ice.`default.dest5` add column new_column Nullable(UInt64);

ALTER TABLE ice.`default.dest5`
    (ADD COLUMN `new_column` Nullable(UInt64))

Query id: 825e8155-ec00-45f0-a54e-63291f46ef10

Ok.

0 rows in set. Elapsed: 0.314 sec. 

Ubuntu-2404-noble-amd64-base :) show create table ice.`default.dest5`;

SHOW CREATE TABLE ice.`default.dest5`

Query id: 0c79176a-55dd-4aa4-a4c4-2125f5cde5fe

   ┌─statement────────────────────────────────────────────────┐
1. │ CREATE TABLE ice.`default.dest5`                        ↴│
   │↳(                                                       ↴│
   │↳    `event_month` Int32,                                ↴│
   │↳    `id` Int64,                                         ↴│
   │↳    `event_time` DateTime64(6),                         ↴│
   │↳    `user_id` Int32,                                    ↴│
   │↳    `category_name` String,                             ↴│
   │↳    `value` Float64,                                    ↴│
   │↳    `payload` String,                                   ↴│
   │↳    `col2` Nullable(Int64),                             ↴│
   │↳    `new_column` Nullable(Int64)                        ↴│
   │↳)                                                       ↴│
   │↳ENGINE = Iceberg('http://localhost:9000/bucket1/dest6/') │
   └──────────────────────────────────────────────────────────┘

DROP COLUMN

 alter table ice.`default.dest5` drop column col2;

ALTER TABLE ice.`default.dest5`
    (DROP COLUMN col2)

Query id: a7905249-670b-4ac2-a626-f95535c02730

Connecting to database ice at localhost:9007 as user default.
Connected to ClickHouse server version 26.6.2.

Ok.

0 rows in set. Elapsed: 0.122 sec. 

Ubuntu-2404-noble-amd64-base :) show create table ice.`default.dest5`;

SHOW CREATE TABLE ice.`default.dest5`

Query id: 9d3b156a-ca6d-4997-917a-8bf36d4d7058

   ┌─statement────────────────────────────────────────────────┐
1. │ CREATE TABLE ice.`default.dest5`                        ↴│
   │↳(                                                       ↴│
   │↳    `event_month` Int32,                                ↴│
   │↳    `id` Int64,                                         ↴│
   │↳    `event_time` DateTime64(6),                         ↴│
   │↳    `user_id` Int32,                                    ↴│
   │↳    `category_name` String,                             ↴│
   │↳    `value` Float64,                                    ↴│
   │↳    `payload` String,                                   ↴│
   │↳    `new_column` Nullable(Int64)                        ↴│
   │↳)                                                       ↴│
   │↳ENGINE = Iceberg('http://localhost:9000/bucket1/dest6/') │
   └──────────────────────────────────────────────────────────┘

1 row in set. Elapsed: 0.016 sec. 

RENAME COLUMN

 alter table ice.`default.dest5` rename column new_column to new_column_2;

ALTER TABLE ice.`default.dest5`
    (RENAME COLUMN new_column TO new_column_2)

Query id: e464f77e-0e3d-4ffc-883e-70db1260327f

Ok.

0 rows in set. Elapsed: 0.169 sec. 

Ubuntu-2404-noble-amd64-base :) show create table ice.`default.dest5`;

SHOW CREATE TABLE ice.`default.dest5`

Query id: 0b4b4e06-7d58-4583-821a-95e27f856ba9

   ┌─statement────────────────────────────────────────────────┐
1. │ CREATE TABLE ice.`default.dest5`                        ↴│
   │↳(                                                       ↴│
   │↳    `event_month` Int32,                                ↴│
   │↳    `id` Int64,                                         ↴│
   │↳    `event_time` DateTime64(6),                         ↴│
   │↳    `user_id` Int32,                                    ↴│
   │↳    `category_name` String,                             ↴│
   │↳    `value` Float64,                                    ↴│
   │↳    `payload` String,                                   ↴│
   │↳    `new_column_2` Nullable(Int64)                      ↴│
   │↳)                                                       ↴│
   │↳ENGINE = Iceberg('http://localhost:9000/bucket1/dest6/') │
   └──────────────────────────────────────────────────────────┘

1 row in set. Elapsed: 0.017 sec. 


@subkanthi subkanthi closed this Aug 6, 2026
@subkanthi subkanthi reopened this Aug 6, 2026
{
switch (type->getTypeId())
{
case TypeIndex::UInt8:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added support for bool

return {"string", true};
case TypeIndex::UUID:
return {"uuid", true};
case TypeIndex::Decimal32:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added support for iceberg decimal types.

@subkanthi

subkanthi commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Decimal support


ALTER TABLE ice.`flowers.sample`
ADD COLUMN dec_col Nullable(Decimal(10,2));

ALTER TABLE ice.`flowers.sample`
    (ADD COLUMN `dec_col` Nullable(Decimal(10, 2)))

Query id: 83a0ae12-2b00-40de-85ba-654daa0d7dc0

Ok.

0 rows in set. Elapsed: 0.203 sec. 

Ubuntu-2404-noble-amd64-base :) show create table ice.`flowers.sample`;

SHOW CREATE TABLE ice.`flowers.sample`

Query id: bb3b8676-9b7e-4000-9b08-2fcb30b65079

   ┌─statement─────────────────────────────────────────────────────────┐
1. │ CREATE TABLE ice.`flowers.sample`                                ↴│
   │↳(                                                                ↴│
   │↳    `value` Nullable(Int64),                                     ↴│
   │↳    `boolean_col` Nullable(Bool),                                ↴│
   │↳    `dec_col` Nullable(Decimal(10, 2))                           ↴│
   │↳)                                                                ↴│
   │↳ENGINE = Iceberg('http://localhost:9000/bucket1/flowers/sample/') │
   └───────────────────────────────────────────────────────────────────┘

1 row in set. Elapsed: 0.009 sec.

@DimensionWieldr

Copy link
Copy Markdown
Collaborator

Context: #2090

Retested with this PR's build.

Code: 290. DB::Exception: Too many unsuccessed retries to alter iceberg table. (LIMIT_EXCEEDED)
(query: ALTER TABLE … DROP COLUMN new_column_…)

Previously this class of failure surfaced as Code 736 (Iceberg alter: catalog commit failed … after metadata file was written successfully). With the retry loop, permanent catalog rejections are retried until the budget is exhausted, so the user-visible code is now 290.

Every retry hits the same HTTP 400:

{"error":{"message":"Invalid last column ID: 41 < 42 (previous last column ID)",
          "type":"IllegalArgumentException","code":400}}

So ClickHouse is proposing a schema update whose last-column-id decreases. Iceberg requires last-column-id to be non-decreasing. On DROP COLUMN (and any schema evolution), is the last-column-id being written incorrectly somewhere?

@mkmkme

mkmkme commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

I asked Claude to analyze this PR and it came with some review. Please have a look: https://gist.github.com/mkmkme/b28a41f136a62d5a6d41a73dee0132bd

@subkanthi

Copy link
Copy Markdown
Collaborator Author

Ubuntu-2404-noble-amd64-base :) alter table ice.`flowers.sample` add column dec_col2 Nullable(Decimal(10,2));

ALTER TABLE ice.`flowers.sample`
    (ADD COLUMN `dec_col2` Nullable(Decimal(10, 2)))

Query id: 37668c59-de09-4b90-84ec-a542e702b05a

Ok.

0 rows in set. Elapsed: 0.249 sec. 

Ubuntu-2404-noble-amd64-base :) show create table ice.`flowers.sample`;

SHOW CREATE TABLE ice.`flowers.sample`

Query id: 54a5faf6-7016-41ab-865d-93f6102f6808

   ┌─statement─────────────────────────────────────────────────────────┐
1. │ CREATE TABLE ice.`flowers.sample`                                ↴│
   │↳(                                                                ↴│
   │↳    `value` Nullable(Int64),                                     ↴│
   │↳    `boolean_col` Nullable(Bool),                                ↴│
   │↳    `dec_col` Nullable(Decimal(10, 2)),                          ↴│
   │↳    `dec_col2` Nullable(Decimal(10, 2))                          ↴│
   │↳)                                                                ↴│
   │↳ENGINE = Iceberg('http://localhost:9000/bucket1/flowers/sample/') │
   └───────────────────────────────────────────────────────────────────┘

1 row in set. Elapsed: 0.012 sec. 

Ubuntu-2404-noble-amd64-base :) alter table ice.`flowers.sample` drop column dec_col2;

ALTER TABLE ice.`flowers.sample`
    (DROP COLUMN dec_col2)

Query id: 49447f08-f164-47a1-aff3-d4852e4736cb

Ok.

0 rows in set. Elapsed: 0.147 sec. 

Ubuntu-2404-noble-amd64-base :) show create table ice.`flowers.sample`;

SHOW CREATE TABLE ice.`flowers.sample`

Query id: 8cd4a422-13d5-472a-9cf2-5208c79e0a99

   ┌─statement─────────────────────────────────────────────────────────┐
1. │ CREATE TABLE ice.`flowers.sample`                                ↴│
   │↳(                                                                ↴│
   │↳    `value` Nullable(Int64),                                     ↴│
   │↳    `boolean_col` Nullable(Bool),                                ↴│
   │↳    `dec_col` Nullable(Decimal(10, 2))                           ↴│
   │↳)                                                                ↴│
   │↳ENGINE = Iceberg('http://localhost:9000/bucket1/flowers/sample/') │
   └───────────────────────────────────────────────────────────────────┘

1 row in set. Elapsed: 0.006 sec. 

Ubuntu-2404-noble-amd64-base :) alter table ice.`flowers.sample` rename column dec_col to dec_col_2;

ALTER TABLE ice.`flowers.sample`
    (RENAME COLUMN dec_col TO dec_col_2)

Query id: 816db79c-7370-4ab2-8a6c-d8535337c2aa

Ok.

0 rows in set. Elapsed: 0.119 sec. 

Ubuntu-2404-noble-amd64-base :) show create table ice.`flowers.sample`;

SHOW CREATE TABLE ice.`flowers.sample`

Query id: b752811b-2a78-4097-ba40-5411e1f2a140

   ┌─statement─────────────────────────────────────────────────────────┐
1. │ CREATE TABLE ice.`flowers.sample`                                ↴│
   │↳(                                                                ↴│
   │↳    `value` Nullable(Int64),                                     ↴│
   │↳    `boolean_col` Nullable(Bool),                                ↴│
   │↳    `dec_col_2` Nullable(Decimal(10, 2))                         ↴│
   │↳)                                                                ↴│
   │↳ENGINE = Iceberg('http://localhost:9000/bucket1/flowers/sample/') │
   └─────────────────────────────────────────────────────────────────

@subkanthi

Copy link
Copy Markdown
Collaborator Author

I asked Claude to analyze this PR and it came with some review. Please have a look: https://gist.github.com/mkmkme/b28a41f136a62d5a6d41a73dee0132bd

@mkmkme can u check again please

@mkmkme

mkmkme commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

I asked Claude to analyze this PR and it came with some review. Please have a look: https://gist.github.com/mkmkme/b28a41f136a62d5a6d41a73dee0132bd

@mkmkme can u check again please

@subkanthi the new one is quite cleaner, but there're still some issues worth fixing: https://gist.github.com/mkmkme/98ffbceed6d86e14339537481a5f162c


}

MetadataGenerator::MetadataGenerator(Poco::JSON::Object::Ptr metadata_object_)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting that dropping a column used in partitioning or ordering has had its behaviour changed, especially that of an ordering column which is now completely rejected.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an iceberg spec:

(Section "Schema Evolution") explicitly forbids this: a column that is part of the current sort order or partition spec cannot be removed without first changing the sort order/partition spec to no longer reference it.

@subkanthi

Copy link
Copy Markdown
Collaborator Author

I asked Claude to analyze this PR and it came with some review. Please have a look: https://gist.github.com/mkmkme/b28a41f136a62d5a6d41a73dee0132bd

@mkmkme can u check again please

@subkanthi the new one is quite cleaner, but there're still some issues worth fixing: https://gist.github.com/mkmkme/98ffbceed6d86e14339537481a5f162c

Thanks @mkmkme , i will take a shot at fixing 1, 2, for #5 Glue catalog we have separate issues which I thought is better to go in a separate PR.
5. A failed schema commit on a non-transactional catalog permanently bricks ALTER on that table — and the new error message actively misleads. For Glue/Unity/Hive, Mutations.cpp writes v<N+1>.metadata.json itself, then calls catalog->updateSchema. If that returns false, the loop retries; but the retry re-reads metadata from the catalog's location (Mutations.cpp:832-859), which still points at N. So it regenerates, targets N+1 again, and writeMetadataFileAndVersionHint returns false because the object already exists (Utils.cpp:300-301). 100 iterations, then:

ALTER TABLE commit kept losing to concurrent modifications after 100 retries

There was no concurrent modification. And the orphan v<N+1>.metadata.json makes every subsequent ALTER on that table repeat this until someone deletes it by hand. The loop structure is pre-existing; the misleading message is new, and the unused cleanup failpoint suggests the real fix was intended.

@subkanthi

Copy link
Copy Markdown
Collaborator Author

I asked Claude to analyze this PR and it came with some review. Please have a look: https://gist.github.com/mkmkme/b28a41f136a62d5a6d41a73dee0132bd

@mkmkme can u check again please

@subkanthi the new one is quite cleaner, but there're still some issues worth fixing: https://gist.github.com/mkmkme/98ffbceed6d86e14339537481a5f162c

Thanks @mkmkme , i will take a shot at fixing 1, 2, for #5 Glue catalog we have separate issues which I thought is better to go in a separate PR. 5. A failed schema commit on a non-transactional catalog permanently bricks ALTER on that table — and the new error message actively misleads. For Glue/Unity/Hive, Mutations.cpp writes v<N+1>.metadata.json itself, then calls catalog->updateSchema. If that returns false, the loop retries; but the retry re-reads metadata from the catalog's location (Mutations.cpp:832-859), which still points at N. So it regenerates, targets N+1 again, and writeMetadataFileAndVersionHint returns false because the object already exists (Utils.cpp:300-301). 100 iterations, then:

ALTER TABLE commit kept losing to concurrent modifications after 100 retries

There was no concurrent modification. And the orphan v<N+1>.metadata.json makes every subsequent ALTER on that table repeat this until someone deletes it by hand. The loop structure is pre-existing; the misleading message is new, and the unused cleanup failpoint suggests the real fix was intended.

Scope
Review of Iceberg ALTER TABLE schema evolution logic: MetadataGenerator, RestCatalog, Mutations, DataLakeConfiguration, FailPoint registration, and associated integration/unit tests.

Confirmed Defects (All Fixed)
High
1. Null-pointer dereference in icebergTypesEqual for complex types

Impact: Server exception on any ALTER TABLE ... MODIFY COLUMN with Array/Map/Tuple types.
File: src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp
Fix: Commit a73db85 -- added null guard before extracting Poco::JSON::Object::Ptr.
Medium
2. isAddColumnApplied false negative for complex types (Array/Map/Tuple)

Impact: Retry after "commit state unknown" would re-apply an already-committed ADD COLUMN, causing a catalog-level conflict or schema corruption for non-primitive columns.
Root cause: icebergTypesEqual performed exact JSON comparison including embedded field IDs. A previous commit allocated IDs from a lower last-column-id, so the same structural type produces a different JSON descriptor on retry.
File: src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp:152-175, 240-261
Fix: Commit 7dc6cfb -- introduced stripNestedFieldIds + icebergTypesEqualIgnoringIds that deep-clones and removes id/element-id/key-id/value-id before comparison. isAddColumnApplied now uses this relaxed comparison.
Test: gtest_iceberg_metadata_generator.cpp -- AddColumnAppliedDetectsCommittedComplexColumn, AddColumnAppliedRejectsComplexTypeMismatch.
3. Schema deduplication in buildUpdateSchemaRequestBody never fires

Impact: Every schema update creates a new schema entry in the schemas array even when the schema is structurally identical to an existing one. This bloats metadata, violates the Iceberg spec's intent for deduplication, and can trigger "too many schemas" limits in some catalogs.
Root cause: schemasEquivalentIgnoringId stripped schema-id but not identifier-field-ids. Schemas committed through the REST catalog carry "identifier-field-ids": [] while freshly generated schemas omit the field entirely. The mismatch caused icebergJsonObjectEquals to always return false.
File: src/Databases/DataLake/RestCatalog.cpp:161, 227-239
Fix: Commit 7dc6cfb -- schemasEquivalentIgnoringId now also strips identifier-field-ids when it is null or an empty array, treating absent-vs-empty as semantically identical.
Test: gtest_rest_catalog_update_metadata.cpp -- EquivalentSchemaDeduplicatesAcrossIdentifierFieldIds, NonEmptyIdentifierFieldIdsPreventDeduplication.
4. MODIFY COLUMN on geometry types throws a misleading error

Impact: ALTER TABLE ... MODIFY COLUMN g Geometry on an existing geometry column throws "Using geometry/geography types is not allowed without allow_experimental_geo_types_in_iceberg flag" even when the column already has that type (should be a no-op).
Root cause: MetadataGenerator hard-coded allow_geo_parser = false in calls to getSimpleType/getIcebergType, so geometry type lookup always failed regardless of the user's setting.
File: src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h:20, Mutations.cpp:875
Fix: Commit 5968163 -- MetadataGenerator constructor now accepts bool allow_geo_parser_ and threads it to type-resolution calls. Mutations.cpp passes context->getSettingsRef()[Setting::allow_experimental_geo_types_in_iceberg].
Low
5. Wrong doc comment on buildUpdateSchemaRequestBody

Impact: Misleading for developers; no runtime effect.
File: src/Databases/DataLake/RestCatalog.h:248-258
Fix: Commit 846e827 -- replaced with an accurate description of schema-update body construction.
6. Dead code: buildUpdateMetadataRequestBody null-snapshot check

Impact: Unreachable branch; misleading for maintainers.
File: src/Databases/DataLake/RestCatalog.cpp (caller throws NOT_IMPLEMENTED before the builder is reached).
Fix: Commit 5968163 -- removed the dead if (!new_snapshot) return nullptr; branch.
7. Dead failpoint: iceberg_alter_orphan_metadata_cleanup_fail

Impact: Registered in FailPoint.cpp with no injection site anywhere -- leftover from unimplemented cleanup logic.
Fix: Commit 5968163 -- removed registration entirely.
8. Misnamed failpoint: iceberg_alter_catalog_update_metadata_fail

Impact: Injected in updateSchema, not updateMetadata; no test exercised it.
Fix: Commit 5968163 -- renamed to iceberg_alter_catalog_update_schema_fail.
9. Unreachable failpoint datalake_iceberg_metadata_create_fail with weakened invariant

Impact: The failpoint fabricated an impossible state (null metadata after create), and assertInitialized was downgraded from LOGICAL_ERROR to NOT_INITIALIZED solely to let the test assert on it. This lost CI/fuzzer signal.
Fix: Commit 5968163 -- removed the failpoint, removed the test test_writes_when_metadata_is_not_initialized, restored LOGICAL_ERROR in assertInitialized.
10. Stale test assertion in test_writes_modify_column.py

Impact: The overly broad assertion ("bad cast" in el or "can not convert" in el or ...) would pass even if the error message regressed to an unrelated exception.
Fix: Commit 5968163 -- tightened to assert "doesn't allow schema evolution" in el.
Residual Risks / Not Tested
Signed overflow in getNextSchemaId -- Returns max_id + 1 where max_id is Int32. Overflow at INT32_MAX schemas is theoretically possible but requires ~2 billion schema evolutions -- not a realistic production scenario.
Retry logic for MODIFY COLUMN -- No isModifyColumnApplied predicate exists. A "commit state unknown" during MODIFY cannot be safely retried without it. Low risk: MODIFY is rare and type widening is idempotent in Iceberg.
deepCopy via stringify/parse -- Used in stripNestedFieldIds path. Allocates and parses JSON on every isAddColumnApplied call. Acceptable for ALTER frequency but would be problematic if ever called in a hot path.
Summary
10 findings total: 1 High, 4 Medium, 5 Low.
All confirmed defects are fixed across commits a73db85, 846e827, 5968163, c72983e, 7dc6cfb.
New unit tests cover the two medium-severity fixes (complex-type retry detection, schema deduplication).
Integration test assertions tightened for regression coverage.
Confidence: High -- all fixes verified by compilation and unit test execution; logic validated by code inspection against the Iceberg spec.```

@mkmkme

mkmkme commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Could you fix the conflicts please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants